domain: allow concurrent user-land impl #33013

Trott · 2020-04-23T00:19:33Z

Picking up #26326

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Trott · 2020-04-23T00:21:00Z

A chunk of code that was deleted in the original pull request is restored here so that test/parallel/test-domain-emit-error-handler-stack.js will pass. Is that the right approach, or is the thing to do to delete/update the test as its behavior should change?

@misterdjules @vdeturckheim @addaleax @devsnek

Trott · 2020-04-23T00:21:36Z

Remaining to be done:

Need to document isDomainLike symbol for end users
Need to include use of isDomainLike symbol by end users in a test
Proof-of-concept code for .isDomainLike by @misterdjules is at https://github.com/misterdjules/domaine/blob/a6b928c906245dd7a1d93b581c6286b298dfdc92/index.js#L178

mscdex · 2020-04-23T03:40:36Z

This produces some significant performance regressions:

                                                    confidence improvement accuracy (*)   (**)  (***)
events/ee-emit.js listeners=10 argc=0 n=2000000            ***    -25.42 %       ±1.07% ±1.43% ±1.88%
events/ee-emit.js listeners=10 argc=10 n=2000000           ***    -17.68 %       ±0.89% ±1.18% ±1.53%
events/ee-emit.js listeners=10 argc=2 n=2000000            ***    -14.36 %       ±2.09% ±2.81% ±3.71%
events/ee-emit.js listeners=10 argc=4 n=2000000            ***    -14.72 %       ±1.41% ±1.89% ±2.47%
events/ee-emit.js listeners=1 argc=0 n=2000000             ***    -19.82 %       ±3.42% ±4.56% ±5.94%
events/ee-emit.js listeners=1 argc=10 n=2000000            ***    -34.73 %       ±2.36% ±3.15% ±4.12%
events/ee-emit.js listeners=1 argc=2 n=2000000             ***    -32.23 %       ±3.10% ±4.14% ±5.43%
events/ee-emit.js listeners=1 argc=4 n=2000000             ***    -27.68 %       ±2.74% ±3.66% ±4.79%
events/ee-emit.js listeners=5 argc=0 n=2000000             ***    -14.22 %       ±1.69% ±2.25% ±2.94%
events/ee-emit.js listeners=5 argc=10 n=2000000            ***    -20.74 %       ±1.79% ±2.40% ±3.15%
events/ee-emit.js listeners=5 argc=2 n=2000000             ***    -13.06 %       ±1.46% ±1.95% ±2.54%
events/ee-emit.js listeners=5 argc=4 n=2000000             ***    -14.43 %       ±2.13% ±2.85% ±3.72%
events/ee-once.js argc=4 n=20000000                        ***     -4.64 %       ±1.85% ±2.48% ±3.25%
events/ee-once.js argc=5 n=20000000                        ***     -4.46 %       ±1.60% ±2.13% ±2.78%

Can we find a way to avoid these first?

addaleax · 2020-04-24T17:04:26Z

I’m surprised that the impact is that large, but yes, we should definitely address that first…

Trott · 2020-08-07T15:47:23Z

@mscdex Can you re-run the benchmark CI so I can see what parameters you use / it uses? I'm running locally and getting very different results, which may be correct--different platforms/environments having different benchmarks--but it seems striking.

 events/ee-add-remove.js n=1000000                                         0.93 %       ±3.44%  ±4.58%  ±5.97%
 events/ee-emit.js listeners=1 argc=0 n=2000000                            1.63 %      ±13.38% ±17.81% ±23.20%
 events/ee-emit.js listeners=1 argc=10 n=2000000                          -1.59 %      ±14.54% ±19.35% ±25.20%
 events/ee-emit.js listeners=1 argc=2 n=2000000                           -3.36 %      ±13.41% ±17.84% ±23.22%
 events/ee-emit.js listeners=1 argc=4 n=2000000                            0.36 %      ±13.42% ±17.85% ±23.24%
 events/ee-emit.js listeners=10 argc=0 n=2000000                           0.03 %      ±14.22% ±18.92% ±24.63%
 events/ee-emit.js listeners=10 argc=10 n=2000000                         -0.16 %      ±17.77% ±23.64% ±30.77%
 events/ee-emit.js listeners=10 argc=2 n=2000000                          -4.87 %      ±14.30% ±19.03% ±24.77%
 events/ee-emit.js listeners=10 argc=4 n=2000000                          -1.99 %      ±13.51% ±17.98% ±23.42%
 events/ee-emit.js listeners=5 argc=0 n=2000000                           -7.17 %      ±15.48% ±20.60% ±26.81%
 events/ee-emit.js listeners=5 argc=10 n=2000000                          -9.15 %      ±12.54% ±16.68% ±21.71%
 events/ee-emit.js listeners=5 argc=2 n=2000000                           -4.81 %      ±14.36% ±19.11% ±24.88%
 events/ee-emit.js listeners=5 argc=4 n=2000000                           -8.17 %      ±13.48% ±17.94% ±23.37%
 events/ee-listener-count-on-prototype.js n=50000000                       0.18 %       ±3.31%  ±4.41%  ±5.75%
 events/ee-listeners.js raw='false' listeners=5 n=5000000                 -0.98 %       ±3.63%  ±4.83%  ±6.29%
 events/ee-listeners.js raw='false' listeners=50 n=5000000                 2.08 %       ±3.95%  ±5.25%  ±6.83%
 events/ee-listeners.js raw='true' listeners=5 n=5000000                   0.06 %       ±3.60%  ±4.79%  ±6.25%
 events/ee-listeners.js raw='true' listeners=50 n=5000000           *      8.66 %       ±6.59%  ±8.77% ±11.43%
 events/ee-once.js argc=0 n=20000000                                      -3.79 %      ±11.45% ±15.26% ±19.92%
 events/ee-once.js argc=1 n=20000000                                      -3.11 %       ±7.93% ±10.55% ±13.74%
 events/ee-once.js argc=4 n=20000000                                      -2.75 %       ±8.40% ±11.18% ±14.56%
 events/ee-once.js argc=5 n=20000000                                       2.38 %       ±8.14% ±10.83% ±14.09%
 events/eventtarget.js listeners=1 n=1000000                        *     -6.82 %       ±6.76%  ±9.00% ±11.73%
 events/eventtarget.js listeners=10 n=1000000                              5.46 %       ±6.92%  ±9.21% ±12.01%
 events/eventtarget.js listeners=5 n=1000000                      ***    -17.82 %       ±5.94%  ±7.91% ±10.29%

I also ran this after rebasing against master so it's possible something else changed that caused this to have less impact.

nodejs-github-bot · 2020-08-07T18:46:20Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32674/

mscdex · 2020-08-08T00:34:08Z

CI results look about the same as before for this PR:

                                                          confidence improvement accuracy (*)   (**)   (***)
events/ee-emit.js listeners=10 argc=0 n=2000000                  ***    -22.05 %       ±1.25% ±1.67%  ±2.18%
events/ee-emit.js listeners=10 argc=10 n=2000000                 ***    -15.45 %       ±2.06% ±2.76%  ±3.62%
events/ee-emit.js listeners=10 argc=2 n=2000000                  ***     -8.32 %       ±1.84% ±2.47%  ±3.26%
events/ee-emit.js listeners=10 argc=4 n=2000000                  ***     -9.01 %       ±0.89% ±1.18%  ±1.54%
events/ee-emit.js listeners=1 argc=0 n=2000000                   ***    -15.22 %       ±5.73% ±7.67% ±10.09%
events/ee-emit.js listeners=1 argc=10 n=2000000                  ***    -31.82 %       ±4.32% ±5.80%  ±7.67%
events/ee-emit.js listeners=1 argc=2 n=2000000                   ***    -26.61 %       ±2.35% ±3.14%  ±4.09%
events/ee-emit.js listeners=1 argc=4 n=2000000                   ***    -30.98 %       ±3.45% ±4.62%  ±6.09%
events/ee-emit.js listeners=5 argc=0 n=2000000                   ***    -15.69 %       ±3.59% ±4.79%  ±6.26%
events/ee-emit.js listeners=5 argc=10 n=2000000                  ***    -26.00 %       ±1.17% ±1.55%  ±2.02%
events/ee-emit.js listeners=5 argc=2 n=2000000                   ***     -8.70 %       ±1.45% ±1.93%  ±2.51%
events/ee-emit.js listeners=5 argc=4 n=2000000                   ***    -12.15 %       ±1.71% ±2.27%  ±2.96%
events/ee-once.js argc=1 n=20000000                              ***     -4.73 %       ±1.82% ±2.42%  ±3.16%
events/ee-once.js argc=4 n=20000000                              ***     -6.53 %       ±2.73% ±3.64%  ±4.75%
events/ee-once.js argc=5 n=20000000                              ***     -5.82 %       ±3.13% ±4.18%  ±5.48%

Trott · 2020-08-08T14:15:44Z

The parameters for the benchmark CI run, for when that link expires:

nodejs-github-bot · 2021-03-27T04:47:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/37013/

nodejs-github-bot · 2021-03-27T13:37:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/37020/ ✅

mscdex · 2021-03-27T19:55:22Z

FWIW there are still some significant regressions:

events/ee-emit.jslisteners=10 argc=0 n=2000000       ***    -19.07 %       ±1.58% ±2.10%  ±2.74%
events/ee-emit.jslisteners=10 argc=10 n=2000000      ***    -33.23 %       ±0.88% ±1.17%  ±1.52%
events/ee-emit.jslisteners=10 argc=2 n=2000000       ***    -23.71 %       ±0.76% ±1.02%  ±1.33%
events/ee-emit.jslisteners=10 argc=4 n=2000000       ***    -27.12 %       ±0.86% ±1.14%  ±1.49%
events/ee-emit.jslisteners=1 argc=0 n=2000000        ***    -20.25 %       ±3.37% ±4.50%  ±5.88%
events/ee-emit.jslisteners=1 argc=10 n=2000000       ***    -29.84 %       ±4.91% ±6.56%  ±8.58%
events/ee-emit.jslisteners=1 argc=2 n=2000000        ***    -27.89 %       ±2.60% ±3.46%  ±4.53%
events/ee-emit.jslisteners=1 argc=4 n=2000000        ***    -30.05 %       ±3.50% ±4.65%  ±6.06%
events/ee-emit.jslisteners=5 argc=0 n=2000000        ***    -13.61 %       ±1.67% ±2.23%  ±2.90%
events/ee-emit.jslisteners=5 argc=10 n=2000000       ***    -42.20 %       ±1.41% ±1.89%  ±2.50%
events/ee-emit.jslisteners=5 argc=2 n=2000000        ***    -31.84 %       ±1.39% ±1.85%  ±2.42%
events/ee-emit.jslisteners=5 argc=4 n=2000000        ***    -33.71 %       ±1.52% ±2.03%  ±2.66%
events/ee-once.jsargc=0 n=20000000                   ***     -2.04 %       ±0.93% ±1.23%  ±1.61%
events/ee-once.jsargc=5 n=20000000                   ***     -4.53 %       ±1.56% ±2.09%  ±2.73%

Trott · 2021-03-28T04:37:19Z

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/987/

Trott · 2021-03-28T05:24:40Z

Whoops, @mscdex's results were from a benchmark CI run and not a local run, so my benchmark CI run was superfluous. Oh well, here are the results anyway:

22:12:45                                                          confidence improvement accuracy (*)   (**)  (***)
22:12:45 events/ee-add-remove.jsn=1000000                                         0.03 %       ±2.21% ±2.94% ±3.83%
22:12:45 events/ee-emit.jslisteners=10 argc=0 n=2000000                  ***    -17.66 %       ±2.83% ±3.81% ±5.04%
22:12:45 events/ee-emit.jslisteners=10 argc=10 n=2000000                 ***    -32.48 %       ±1.21% ±1.62% ±2.12%
22:12:45 events/ee-emit.jslisteners=10 argc=2 n=2000000                  ***    -24.41 %       ±1.02% ±1.36% ±1.77%
22:12:45 events/ee-emit.jslisteners=10 argc=4 n=2000000                  ***    -26.14 %       ±2.73% ±3.67% ±4.84%
22:12:45 events/ee-emit.jslisteners=1 argc=0 n=2000000                   ***    -20.79 %       ±3.38% ±4.52% ±5.93%
22:12:45 events/ee-emit.jslisteners=1 argc=10 n=2000000                  ***    -34.28 %       ±3.47% ±4.64% ±6.09%
22:12:45 events/ee-emit.jslisteners=1 argc=2 n=2000000                   ***    -27.86 %       ±2.94% ±3.92% ±5.10%
22:12:45 events/ee-emit.jslisteners=1 argc=4 n=2000000                   ***    -28.60 %       ±3.69% ±4.92% ±6.43%
22:12:45 events/ee-emit.jslisteners=5 argc=0 n=2000000                   ***    -11.99 %       ±1.83% ±2.44% ±3.17%
22:12:45 events/ee-emit.jslisteners=5 argc=10 n=2000000                  ***    -41.38 %       ±1.29% ±1.73% ±2.27%
22:12:45 events/ee-emit.jslisteners=5 argc=2 n=2000000                   ***    -31.60 %       ±2.03% ±2.73% ±3.60%
22:12:45 events/ee-emit.jslisteners=5 argc=4 n=2000000                   ***    -33.87 %       ±2.17% ±2.91% ±3.84%
22:12:45 events/ee-listener-count-on-prototype.jsn=50000000                       2.23 %       ±4.07% ±5.44% ±7.12%
22:12:45 events/ee-listeners.jsraw='false' listeners=50 n=5000000                -0.15 %       ±2.52% ±3.36% ±4.39%
22:12:45 events/ee-listeners.jsraw='false' listeners=5 n=5000000                  1.06 %       ±1.55% ±2.07% ±2.69%
22:12:45 events/ee-listeners.jsraw='true' listeners=50 n=5000000                 -0.02 %       ±1.82% ±2.43% ±3.16%
22:12:45 events/ee-listeners.jsraw='true' listeners=5 n=5000000                   1.66 %       ±3.92% ±5.22% ±6.82%
22:12:45 events/ee-once.jsargc=0 n=20000000                                      -1.25 %       ±3.90% ±5.19% ±6.76%
22:12:45 events/ee-once.jsargc=1 n=20000000                               **     -4.86 %       ±2.93% ±3.95% ±5.23%
22:12:45 events/ee-once.jsargc=4 n=20000000                              ***     -5.65 %       ±1.54% ±2.06% ±2.68%
22:12:45 events/ee-once.jsargc=5 n=20000000                              ***     -5.21 %       ±0.96% ±1.29% ±1.69%
22:12:45 events/eventtarget.jslisteners=10 n=1000000                              0.02 %       ±1.75% ±2.32% ±3.02%
22:12:45 events/eventtarget.jslisteners=1 n=1000000                              -2.71 %       ±3.96% ±5.33% ±7.06%
22:12:45 events/eventtarget.jslisteners=5 n=1000000                              -0.94 %       ±3.50% ±4.69% ±6.16%
22:12:45 
22:12:45 Be aware that when doing many comparisons the risk of a false-positive
22:12:45 result increases. In this case, there are 25 comparisons, you can thus
22:12:45 expect the following amount of false-positive results:
22:12:45   1.25 false positives, when considering a   5% risk acceptance (*, **, ***),
22:12:45   0.25 false positives, when considering a   1% risk acceptance (**, ***),
22:12:45   0.03 false positives, when considering a 0.1% risk acceptance (***)
22:12:45 ++ mv output280321-054823.csv /w/bnch-comp

Currently, only one domain-lke implementation (the core domain one) can be used to handle uncaught exceptions or unhandled error events. This PR aims at making it possible for different domain-like user-land implementations to be used concurrently (including with the core domain impl) so that the state of the core domain module (doc deprecated) does not prevent users of domains from having a well-maintained domain-like facility. Ref: nodejs#23348 Co-authored-by: Rich Trott <rtrott@gmail.com>

Trott · 2021-03-31T15:38:47Z

I'm not seeing any obvious way to improve performance and this isn't something I'm inclined to seek out non-obvious ways to do it. So I'm inclined to close.

@mscdex Do you see anything obvious that might be worth trying to refactor into a more performant shape?

nodejs-github-bot added domain Issues and PRs related to the domain subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. events Issues and PRs related to the events subsystem / EventEmitter. labels Apr 23, 2020

misterdjules mentioned this pull request May 4, 2020

domain: allow concurrent user-land impl #26326

Closed

4 tasks

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18

Trott force-pushed the support-userland-domains-bis branch from e3a3916 to 9fb161c Compare August 7, 2020 05:39

mmarchini added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2020

github-actions bot mentioned this pull request Aug 8, 2020

CI Relability 2020-08-08 JaneaSystems/nodejs-reliability#62

Open

12 tasks

github-actions bot mentioned this pull request Aug 8, 2020

CI Reliability 2020-08-08 nodejs/reliability#37

Open

12 tasks

github-actions bot mentioned this pull request Aug 14, 2020

CI Reliability 2020-08-14 nodejs/reliability#43

Open

6 tasks

Trott force-pushed the support-userland-domains-bis branch from 9fb161c to 134cbe1 Compare March 27, 2021 04:43

Trott force-pushed the support-userland-domains-bis branch 2 times, most recently from f2ae64b to 6cc7faa Compare March 27, 2021 13:37

Trott force-pushed the support-userland-domains-bis branch from 6cc7faa to a45ef63 Compare March 28, 2021 04:42

guybedford force-pushed the master branch from dc5a5da to 8e46568 Compare March 29, 2021 21:33

Julien Gilli and others added 2 commits March 30, 2021 06:44

event: simplify domain conditions

110983c

Trott force-pushed the support-userland-domains-bis branch from a45ef63 to 110983c Compare March 30, 2021 17:50

Trott added 2 commits March 30, 2021 22:00

fixup! event: simplify domain conditions

77fd57d

fixup! fixup! event: simplify domain conditions

f992e36

Trott force-pushed the support-userland-domains-bis branch from 8e77095 to f992e36 Compare March 31, 2021 14:24

Trott closed this Mar 31, 2021

Trott deleted the support-userland-domains-bis branch September 25, 2022 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

domain: allow concurrent user-land impl #33013

domain: allow concurrent user-land impl #33013

Trott commented Apr 23, 2020

Trott commented Apr 23, 2020

Trott commented Apr 23, 2020

mscdex commented Apr 23, 2020

addaleax commented Apr 24, 2020

Trott commented Aug 7, 2020 •

edited

Loading

nodejs-github-bot commented Aug 7, 2020

mscdex commented Aug 8, 2020 •

edited

Loading

Trott commented Aug 8, 2020

nodejs-github-bot commented Mar 27, 2021

nodejs-github-bot commented Mar 27, 2021 •

edited by Trott

Loading

mscdex commented Mar 27, 2021

Trott commented Mar 28, 2021

Trott commented Mar 28, 2021

Trott commented Mar 31, 2021

domain: allow concurrent user-land impl #33013

domain: allow concurrent user-land impl #33013

Conversation

Trott commented Apr 23, 2020

Checklist

Trott commented Apr 23, 2020

Trott commented Apr 23, 2020

mscdex commented Apr 23, 2020

addaleax commented Apr 24, 2020

Trott commented Aug 7, 2020 • edited Loading

nodejs-github-bot commented Aug 7, 2020

mscdex commented Aug 8, 2020 • edited Loading

Trott commented Aug 8, 2020

nodejs-github-bot commented Mar 27, 2021

nodejs-github-bot commented Mar 27, 2021 • edited by Trott Loading

mscdex commented Mar 27, 2021

Trott commented Mar 28, 2021

Trott commented Mar 28, 2021

Trott commented Mar 31, 2021

Trott commented Aug 7, 2020 •

edited

Loading

mscdex commented Aug 8, 2020 •

edited

Loading

nodejs-github-bot commented Mar 27, 2021 •

edited by Trott

Loading